Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

issue-50839 handle large int write input. #53338

Merged
merged 6 commits into from
Jul 30, 2021
Merged

issue-50839 handle large int write input. #53338

merged 6 commits into from
Jul 30, 2021

Conversation

lateapexearlyspeed
Copy link
Contributor

Fix #50839 , use uint arithmetic operation should be enough to handle mentioned case.

@ghost
Copy link

ghost commented May 27, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #50839 , use uint arithmetic operation should be enough to handle mentioned case.

Author: lateapexearlyspeed
Assignees: -
Labels:

area-System.IO
Milestone: -

@lateapexearlyspeed lateapexearlyspeed mentioned this pull request May 27, 2021
Closed
This was referenced May 27, 2021
Open
Closed
This was referenced Jun 1, 2021
Closed
Closed
Closed
stephentoub
stephentoub reviewed Jun 26, 2021
View reviewed changes
src/libraries/System.IO/tests/BufferedStream/BufferedStreamTests.cs Outdated
@@ -12,6 +12,8 @@ namespace System.IO.Tests
{
public class BufferedStream_StreamAsync
{
public static bool IsX64 { get; } = IntPtr.Size >= 8;
Copy link
Member

@stephentoub stephentoub Jun 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary... you can use this (as a theory):

runtime/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/NFloatTests.cs

Line 43 in 57bfe47

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

stephentoub
stephentoub approved these changes Jun 26, 2021
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor issues, otherwise LGTM. Thanks.

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley jeffhandley requested a review from adamsitnik July 23, 2021 21:16
@jeffhandley
Copy link
Member

@lateapexearlyspeed There's some minor feedback to address on this PR. We'll be ready to re-review if you can get the PR updated in the next week or two; after that, this would most likely slip out to .NET 7.0.

@adamsitnik This PR is assigned to you for follow-up/decision before the RC1 snap.

@lateapexearlyspeed
Copy link
Contributor Author

Hi @jeffhandley and @adamsitnik , all comments are fixed now.

@jeffhandley
Copy link
Member

Thanks, @lateapexearlyspeed. I just pushed a commit to fix one remaining spacing nit-pick. After the checks all run, I'll take another look to ensure everything is passing.

@jeffhandley
Copy link
Member

I'm going to push a merge from main to get another run with the merged bits.

@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@jeffhandley
Copy link
Member

The failed staging and installer checks looked transitory and unrelated; re-running them just to be certain.

@jeffhandley jeffhandley merged commit 5058ca8 into dotnet:main Jul 30, 2021
@adamsitnik adamsitnik added this to the 6.0.0 milestone Jul 30, 2021
@MihaZupan MihaZupan mentioned this pull request Jul 30, 2021
Merged
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Open
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@jeffhandley jeffhandley jeffhandley approved these changes

@adamsitnik adamsitnik Awaiting requested review from adamsitnik

Assignees

@jeffhandley jeffhandley

Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Calling BufferedStream.Write(array, offset, count) where count > 1073741823 results in an OverflowException
5 participants
@lateapexearlyspeed @jeffhandley @stephentoub @terrajobst @adamsitnik